Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add Big Number support #40

Open
wants to merge 11 commits into
base: master
Choose a base branch
from

Conversation

SebastianG77
Copy link

@SebastianG77 SebastianG77 commented Feb 10, 2019

Hi,

I need to diff JSON files which contain large numbers. Even though your module is generally very useful for comparing JSON files, it is unable to create proper diffs for JSON files containting large numbers, as it parses them by the ordinary JSON.parse() function. As a result, when comparing two files having the following JSON objects, the diff will not be recognized by this module:

file1:
{
"value": 3e+6000
}

file2:
{
"value": 3e+5000
}

However, I added optional BigNumber support. So if you now compare these two files and add the parameter -b, you will get the following diff:

{
- value: 3e+6000
+ value: 3e+5000
}

I know this is a very special use case as JSON objects usually do not have big numbers, but this feature is still useful as it extends the use of this module. It also should not break anything as my changes will only be activated if the options object contains the property bigNumberSupport. Moreover, all previous test cases are still running. I also added some additional test cases to check if this feature works and I am willing to add some more if desired.

Please let me know soon, if you are intersted in merging this pull request, as I also personally need this feature.

Otherwise I at least would like to publish the fork to npm by myself if you don't mind.

Looking forward for some response.

@SebastianG77
Copy link
Author

Hi,

I just wanted to let you know that I now published my fork at npm (https://www.npmjs.com/package/big-json-diff).

I hope this is okay for you. I still will appreciate if you merge my pull request.

@ewoudenberg
Copy link
Collaborator

ewoudenberg commented Mar 6, 2022

Hi @SebastianG77 ! Thank you, this seems like a useful change. However, we've converted our codebase over to ES6 and also cleaned up the way options are handled in index.js. If you can implement your change against the current codebase we can merge it in.

Copy link
Collaborator

@ewoudenberg ewoudenberg left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @SebastianG77 ! Thank you, this seems like a useful change. However, we've converted our codebase over to ES6 and also cleaned up the way options are handled in index.js. If you can implement your change against the current codebase we can merge it in.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants